HDDS-15175. Introduce StorageClass and storage policy#10191
HDDS-15175. Introduce StorageClass and storage policy#10191xichen01 wants to merge 5 commits intoapache:HDDS-11233from
Conversation
chungen0126
left a comment
chungen0126
left a comment
There was a problem hiding this comment.
Thanks @xichen01 for the patch. Overall, looks good to me, but I have a few minor suggestions.
| return StoragePolicyProto.COLD; | ||
| default: | ||
| throw new IllegalArgumentException( | ||
| "Error: StoragePolicyType not found, type=" + this); |
There was a problem hiding this comment.
nit: The class name is OzoneStoragePolicy.
| "Error: StoragePolicyType not found, type=" + this); | |
| "Error: OzoneStoragePolicy not found, type=" + this); |
| if (storageTypes.isEmpty()) { | ||
| return Collections.emptyList(); | ||
| } | ||
| return new ArrayList<>(Collections.nCopies(numberOfNodes, storageTypes.get(0))); |
There was a problem hiding this comment.
Should we consider making the StorageType list read-only to ensure immutability?
| return new ArrayList<>(Collections.nCopies(numberOfNodes, storageTypes.get(0))); | |
| return Collections.unmodifiableList(new ArrayList<>(Collections.nCopies(numberOfNodes, storageTypes.get(0))); |
| if (Arrays.stream(storageTypes).distinct().count() <= 1) { | ||
| throw new IllegalArgumentException("StorageTier '" + tierName + | ||
| "' requires at least two different StorageType instances." + | ||
| " but only " + Arrays.stream(storageTypes).distinct().count() + | ||
| " StorageType were provided."); | ||
| } |
There was a problem hiding this comment.
While I understand that non-uniform storage tiers aren't supported yet (so this line might not be reachable), Arrays.stream(storageTypes).distinct().count() is called twice here. We could assign it to a variable instead.
| if (Arrays.stream(storageTypes).distinct().count() <= 1) { | |
| throw new IllegalArgumentException("StorageTier '" + tierName + | |
| "' requires at least two different StorageType instances." + | |
| " but only " + Arrays.stream(storageTypes).distinct().count() + | |
| " StorageType were provided."); | |
| } | |
| long distinctStorageTypes = Arrays.stream(storageTypes).distinct().count(); | |
| if (distinctStorageTypes <= 1) { | |
| throw new IllegalArgumentException("StorageTier '" + tierName + | |
| "' requires at least two different StorageType instances." + | |
| " but only " + distinctStorageTypes + | |
| " StorageType were provided."); | |
| } |
There was a problem hiding this comment.
Yes, non-uniform storage tiers are not currently supported; this is merely a reserved interface.
| import org.apache.hadoop.hdds.client.StorageTier; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class StoragePolicyTest { |
There was a problem hiding this comment.
Could you please add some unit tests to cover the Protobuf/Java conversion?
There was a problem hiding this comment.
Added a test for this
| * Ozone specific storage tiers. | ||
| */ | ||
| public enum StorageTier { | ||
| SSD("SSD", StorageType.SSD), |
There was a problem hiding this comment.
Do you think we may introduce the NVME storage type (here and below where it's needed)?
There was a problem hiding this comment.
I think it depends on the actual needs. If you need to differentiate between regular low-speed SATA/SAS SSDs and high-speed NVMe SSDs, you can add an NVMe SSD.
You can create a subtask in https://issues.apache.org/jira/browse/HDDS-11233, and then we can submit a separate PR to add this NVME storage type.
FYI: Since the subsequent code is based on the current storage types, it is recommended to add this feature later; otherwise, it may introduce some conflicts.
|
LGTM |
4 participants
What changes were proposed in this pull request?
Introduce StorageClass and storage policy, introducing storage policy related definitions
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15175
How was this patch tested?
unit test